-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add functions to compute time resolution periods and matrices #144
Conversation
ac2aa20
to
e4aa78a
Compare
Codecov ReportAll modified lines are covered by tests ✅
... and 2 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
The coverage fails are from input_tables.jl, which I have not touched. We haven't discussed a coverage acceptance criteria, but either way, it is not because of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks looks great! I think Diego will have to check the second function.
src/time-resolution.jl
Outdated
time_step in time_steps | ||
] | ||
|
||
return M |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to call it M (which I assume matches the paper) or something more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna change to matrix
, although it is not going to be visible anywhere outside this function.
e4aa78a
to
9733a81
Compare
src/time-resolution.jl
Outdated
Notice that this implies that they form a disjunct partition of `1:N`. | ||
|
||
The output will also be an array of ranges with the conditions above. | ||
The output is constructed greedly, i.e., it selects the next largest breakpoint following the algorithm below: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"greedily"
time_steps1 = [1:4, 5:8, 9:12] | ||
time_steps2 = [1:3, 4:6, 7:9, 10:12] | ||
compute_rp_periods([time_steps1, time_steps2]) | ||
|
||
# output | ||
|
||
3-element Vector{UnitRange{Int64}}: | ||
1:4 | ||
5:8 | ||
9:12 | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more complex example that would explain more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a more complex example, but I don't think it explains more. Let me know what would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Might still want Diego to check it over.
) | ||
period_end = maximum(breakpoints) | ||
@assert period_end ≥ period_start | ||
push!(rp_periods, period_start:period_end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just read what 'push!' does, but won't this link the values with period_start & period_end so they'll update as you change them?
So instead of [1:3, 4:6] you'd get [4:6, 4:6]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. That might happen if the period_start
variable was an array-like (or pointer)-like object. In which case you would need to make a copy. But in this case that is not necessary.
For instance
L = []
v = ones(3)
push!(L, v) # Now L = [[1.0, 1.0, 1.0]]
v[1] = -1 # Now L = [[-1.0, 1.0, 1.0]]
push!(L, v) # Now L = [[-1.0, 1.0, 1.0], [-1.0, 1.0, 1.0]]
That happens because v
is an array and pushing it into L
pushes the "pointer" to it. Doing push!(L, copy(v))
would fix it.
However, this would work:
L = []
v = [1, 1, 1]
push!(L, v)
v = [-1, 1, 1]
push!(L, v)
Because the second v
is not the same "pointer" as the first. L
has two elements, one is the pointer to the first v
, and the second is the pointer to the second v
.
In other words, pointers are complicated.
Hi Lauren, I've made some modifications. Let me know if there is something that would help more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for adding the complex example - I think it adds some explanation. :)
Pull request details
Describe the changes made in this pull request
One assumption is made: the description of a time period is a range, i.e., an object
start:end
.Creates a function
compute_rp_periods
that receives a list of time steps (such as the time steps of various flows), and computes the periods for that representative period.Creates a function
resolution_matrix
that computes the matrix of a given flow/asset in a given representative period.List of related issues or pull requests
Closes #109
Collaboration confirmation
As a contributor I confirm